-
Notifications
You must be signed in to change notification settings - Fork 186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
frontend Sidebar: Improve performance by moving isSelected logic to the parent component #2638
base: main
Are you sure you want to change the base?
Conversation
6b65c95
to
11382dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
I left some notes.
Can you please leave in the PR description how to test?
@@ -14,7 +14,7 @@ import { SidebarEntry } from './sidebarSlice'; | |||
*/ | |||
export interface SidebarItemProps extends ListItemProps, SidebarEntry { | |||
/** The route name which is selected. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation should be updated as well.
> | ||
<a | ||
class="MuiButtonBase-root MuiListItem-root MuiListItem-gutters MuiListItem-padding MuiListItem-button css-sayhe9-MuiButtonBase-root-MuiListItem-root" | ||
class="MuiButtonBase-root MuiListItem-root MuiListItem-gutters MuiListItem-padding MuiListItem-button Mui-selected css-sayhe9-MuiButtonBase-root-MuiListItem-root" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain why this test result changed? I don't see any mention of why, so I'm wondering if it's intentional or a bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I incorrectly updated the story for this case. It's fixed now
Change logic that determines if the item is selected to the parent component. This allows for better memoization of individual items. Signed-off-by: Oleksandr Dubenko <[email protected]>
With this change only items that were updated will rerender